fix(product_enablement): avoid accidentally disabling products on update #763
+60
−99
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #761
Problems
I realised there were some fundamental issues with the original implementation of the
product_enablement
block for thefastly_service_vcl
andfastly_service_compute
resources (docs).name
attribute to be aComputed
attribute. This would cause the block to be deleted, then recreated for any change the user made to the block, which consequently meant the 'Update' method for the block was never being called (I've now changedname
to no longer beComputed
).false
for each product (I've removed that so now the attributes will be stored in the state file asnull
if they're not set in the user's TF config, which avoids accidental API calls to disable a product).false
. We correct it in this PR and so products not enabled simply show asnull
in the state file.Concerns
My main concern with this PR has been to avoid a breaking interface change, that would consequently require a new major version of the provider to be published.
✅ I don't believe this PR introduces a breaking interface change.
There were two fundamental challenges...
name
to be optional.false
as a default for each product.The first challenge was the change to the
name
attribute, which is nowOptional
rather thanComputed
. To prevent any issues, we additionally setDefault: "products"
on the attribute, which helps to keep it in sync with the oldComputed
value, which we had previously hardcoded to "products" anyway. So there shouldn't be any problems for this point.The second challenge was the move away from setting a default of
false
for all products, and this one is more of a 'grey area' when it comes to the "avoid breaking changes" concern and needs more context to explain...A customer who already has
product_enablement
defined in their Terraform config (i.e. they're using it with an older version of the Fastly Terraform provider) is likely going to have all the products defined in their config (even those products they don't use!). This is something users had to do (i.e. set each product tofalse
) to avoid seeing an unexpected diff after theirterraform apply
(this is because of point 3. above).So although in this PR we've fixed things, such that the user no longer has to set any product other than the ones they are able to enable/disable, because we've also fixed an issue with the Update method never being called (see point 1. above), these older/existing customers might run into a separate scenario.
That separate scenario is triggering a 'disable product' API call in the Update method the next time they run an apply if using a version of the provider with this PR change set in it (remember the Update method isn't being called in the current/published implementation due to a bug that this PR fixes).
An example of this issue would be a customer who has Image Optimizer enabled on their account but who wasn't entitled to enable/disable Image Optimizer. The
terraform apply
would fail when the API returns an error to say (paraphrasing) "sorry, you're not entitled to enable/disable this product".The solution to that problem is for the customer to just remove any unnecessary products from their TF (i.e. only include products they are able to enable/disable) but because this is a change in behaviour and not the interface, it might not be obvious for them to do that.
Workaround?
So the second challenge I just mentioned isn't ideal and could be argued that it would require a new major version, because we'd otherwise have customers suddenly see changes in their plans and it would be unclear why that is suddenly happening (and if our "solution" is for the customer to have to manually make a change to their config, then although we've not changed the exposed interface, we have still broken their build).
To workaround that concern, I've implemented in this PR something we do already in the 'Delete' method, which is to check the error returned by the API and if it's a message that indicates the user isn't entitled to enable/disable the product, we'll just ignore the error and not return it. This way we'll allow the customer's
terraform apply
to finish successfully.Yes, they'll still have things like
image_optimizer = false
unnecessarily set in their config (and it'll look confusing if they know that they actually have the product enabled manually by Fastly support), and of course they can delete those products from the Terraform config whenever they like once they upgrade to this version of the provider, but at least they won't see an error and they should hopefully not experience any breaks in user flows.